Skip to content

Conversation

morrys
Copy link

@morrys morrys commented Dec 30, 2019

Hello to all,
this PR allows to support the @defer directive. Although the implementation is mostly complete, the PR is in draft to discuss it together and evaluate its integration.

In the PR I also created tests and I performed the integration tests with Relay, express-graphql & fetch-multipart-graphql with positive results.

In summary how I implemented defer:

  • @defer has two arguments:
    • if (optional, default true) the fragment is defer when true.
    • label (required) used to distinguish deferred fragments
  • @defer currently provides only FRAGMENT_SPREAD & INLINE_FRAGMENT (Relay) as location, but support for FIELD (Apollo) is commented in the PR
    /*
    in order to support defer on field
    const isFieldDefer = shouldDeferNode(exeContext, selection);
    if (deferDirective && !isFieldDefer) {
    */
    if (deferDirective) {
  • I added resolverResult: ResultResolver new property in ExecutionContext which allows the management of the results to be returned at the end of the execution
  • modified execute function now its return type is AsyncIterable<Promise<ExecutionResult> | PromiseOrValue<ExecutionResult>; . Using isAsyncIterable (result) it is possible to distinguish if there are deferred results.
  • fields that are marked as deferred are resolved after the resolution of the previous result. I also implemented an optimization to avoid solving the same field multiple times.

this is a slow motion gif of how it works.
deferred

Let me know if more information is needed.

Thank you
Lorenzo

@wtrocki
Copy link
Contributor

wtrocki commented Dec 30, 2019

Some additional context from Apollo Defer: apollographql/apollo-server#2700

@morrys
Copy link
Author

morrys commented Dec 31, 2019

Looking at the Relay test schema
https://github.com/facebook/relay/blob/5da3be070283c6dcd42774ba33c1590db65fe3c7/packages/relay-test-utils-internal/testschema.graphql#L13-L16 i noticed that defer is also supported for inline_fragment, the last commit support it and add some tests

@morrys
Copy link
Author

morrys commented Dec 31, 2019

In this branch I created support for @stream:
https://github.com/morrys/graphql-js/tree/support-stream-directive

Some checks and optimizations are missing. But the first tests with Relay were positive:
defer-and-stream-directive

@IvanGoncharov
Copy link
Member

@morrys Thanks for your effort 👍
I didn't have enough time to read through actual code but on the surface, it looks like both #2318 and #2319 add very similar functionality especially since both of them Relay compatible.
Or I'm missing something?

About the next steps for merging this PR please see my answer in #2319:
#2319

@morrys @lilianammmatos Can you please review each other PRs and figure out how to collaborate?

@IvanGoncharov IvanGoncharov added the spec RFC Implementation of a proposed change to the GraphQL specification label Jan 4, 2020
@morrys
Copy link
Author

morrys commented Jan 4, 2020

@IvanGoncharov yes the two PR add the same functionality.
Tomorrow I integrate the branch where I implemented the stream and then I dedicate myself to making a review of @lilianammmatos 's PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec RFC Implementation of a proposed change to the GraphQL specification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants